feat(RHOAIENG-30963): Add support for DSC ConfigMaps #531
Conversation
ruivieira
requested review from
RobGeada,
adolfo-ab,
christinaexyou and
sheltoncyril
ruivieira
added
kind/enhancement
Reviewer's GuideThis PR introduces support for overriding LMEval’s online mode and code execution permissions through a DataScienceCluster ConfigMap. It adds constants and a new helper to read the ConfigMap, refactors CreatePod and generateCmd signatures to accept reconciler context and DSC flags, and applies a precedence logic (ConfigMap → operator defaults → job spec) to determine and log the effective permissions when constructing pods. Sequence diagram for pod creation with DSC ConfigMap precedencesequenceDiagram
participant Reconciler as LMEvalJobReconciler
participant ConfigMap as DataScienceCluster ConfigMap
participant Operator as Operator Defaults
participant Job as LMEvalJob.Spec
participant Pod as Pod
Reconciler->>ConfigMap: getDSCLMEvalSettings()
alt ConfigMap found and valid
ConfigMap-->>Reconciler: allowOnline, allowCodeExecution
else ConfigMap not found/invalid
ConfigMap-->>Reconciler: nil, nil
end
Reconciler->>Operator: get AllowOnline, AllowCodeExecution
Reconciler->>Job: get AllowOnline, AllowCodeExecution
Note right of Reconciler: Precedence: ConfigMap > Operator > Job.Spec
Reconciler->>Pod: CreatePod(..., effective permissions)
Class diagram for LMEvalJobReconciler and ConfigMap integrationclassDiagram
class LMEvalJobReconciler {
+getDSCLMEvalSettings(ctx, log) (*bool, *bool, error)
}
class serviceOptions {
+AllowOnline: bool
+AllowCodeExecution: bool
+PodImage: string
+ImagePullPolicy: string
+DriverPort: int
}
class LMEvalJob {
+Spec: LMEvalJobSpec
+Status: LMEvalJobStatus
}
class LMEvalJobSpec {
+AllowOnline: *bool
+AllowCodeExecution: *bool
+Pod: PodSpec
}
class corev1.ConfigMap {
+Data: map[string]string
}
LMEvalJobReconciler --|> serviceOptions : uses
LMEvalJobReconciler --|> LMEvalJob : manages
LMEvalJobReconciler --> corev1.ConfigMap : reads
LMEvalJob --> LMEvalJobSpec : has
LMEvalJobSpec --> PodSpec : has
Flow diagram for permission resolution logicflowchart TD
A[Start Pod Creation] --> B{DSC ConfigMap exists?}
B -- Yes --> C{Valid allowOnline/allowCodeExecution?}
C -- Yes --> D[Use ConfigMap values]
C -- No --> E[Use operator defaults]
B -- No --> E
D --> F{Job.Spec overrides?}
E --> F
F -- Yes --> G[Apply Job.Spec if allowed by effective permissions]
F -- No --> H[Use effective permissions]
G --> I[Create Pod]
H --> I
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ruivieira - I've reviewed your changes - here's some feedback:
- getDSCLMEvalSettings currently ignores the "opendatahub.io/config-source" annotation and will parse any ConfigMap with the DSC name—add an annotation check so unannotated maps are skipped as your tests expect.
- CreatePod calls getDSCLMEvalSettings on every invocation, causing repeated API requests—consider fetching and caching the DSC settings once per reconciliation and passing them in.
- The permission evaluation for online mode and code execution is embedded deeply in CreatePod, making its signature bulkier—extract that logic into a helper or serviceOptions extension to simplify CreatePod.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- getDSCLMEvalSettings currently ignores the "opendatahub.io/config-source" annotation and will parse any ConfigMap with the DSC name—add an annotation check so unannotated maps are skipped as your tests expect.
- CreatePod calls getDSCLMEvalSettings on every invocation, causing repeated API requests—consider fetching and caching the DSC settings once per reconciliation and passing them in.
- The permission evaluation for online mode and code execution is embedded deeply in CreatePod, making its signature bulkier—extract that logic into a helper or serviceOptions extension to simplify CreatePod.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai Your review does not appear to be correct. |
|
PR image build and manifest generation completed successfully! 📦 PR image: 📦 LMES driver image: 📦 LMES job image: 📦 Guardrails orchestrator image: 🗂️ CI manifests |
|
@ruivieira: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| DefaultBatchSize = "1" | ||
| DefaultDetectDevice = true | ||
| ServiceName = "LMES" | ||
| // DataSienceCluster ConfigMap constants |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sheltoncyril The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
3 participants
Adds support for configuring LMEval online mode and code execution permissions through
DataScienceClusterConfigMaps.See RHOAIENG-30963.
Depends (functionally) on opendatahub-io/opendatahub-operator#2225, but can be merged without it.
Summary by Sourcery
Add support for configuring LMEval online mode and code execution permissions via a DataScienceCluster ConfigMap and update pod creation logic to respect these overrides.
New Features:
Tests: